Fix for git-pkg update with package#21
Conversation
When updating a specific package, need to use --upgrade-package instead of just --update. Adding an extra test case to handle both with and without package. Fixes git-pkgs#20
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Thanks for tracking this down, the --upgrade-package mapping is the right fix for the targeted case and the new test covers it well.
One issue: removing default_flags: [--upgrade] changes what happens when no package is given. With this branch, BuildCommand("uv", "update", CommandInput{}) now produces [uv sync] instead of [uv sync --upgrade]. Bare uv sync only reconciles the venv with the existing lockfile and won't upgrade anything (per the uv docs: "uv will prefer the previously locked versions of packages when running uv sync"). So git-pkgs update with no package would silently become a no-op for uv projects.
The change to TestUvUpdate (adding Flags: map[string]any{"all": true}) hides this. That test was asserting the no-arg contract; nothing in the library or in git-pkgs sets an all flag automatically, so the test should keep passing CommandInput{}.
Unfortunately we can't just keep default_flags: [--upgrade] alongside the new --upgrade-package arg. uv's Upgrade::from_args (source) sets the strategy to All whenever --upgrade is present, so uv sync --upgrade-package requests --upgrade would upgrade everything, not just requests.
The schema can't currently express "apply this default flag only when arg X is absent", so this needs a small translator change rather than a pure YAML fix. Could you:
- Revert
TestUvUpdateto useCommandInput{}and keep asserting[uv sync --upgrade] - Restore
default_flags: [--upgrade]inuv.yaml(and drop theallflag) - Add a
suppresses_default_flags: truefield to theArgschema (definitions/schema.go) and havebuildSingleCommandintranslator.goskip appendingcmd.DefaultFlagswhen any provided arg has it set
That keeps both update and update <pkg> correct and the mechanism is reusable for other managers with the same all-vs-one split (conda has a similar shape).
Happy to pair on the translator bit if that's more than you signed up for, just let me know.
|
Sure, let me give it a shot first, and I'll let you know if I need assistance :) |
There are cases when an argument is passed, that we will want to then skip the default flags. In the example of uv for python, with no args we want to use `uv sync --upgrade` to update everything, but in the case of a single package, we want to do `uv sync --upgrade-package <name>`
| args = append(args, cmd.DefaultFlags...) | ||
| if suppresses_default_flags == false { | ||
| args = append(args, cmd.DefaultFlags...) | ||
| } |
There was a problem hiding this comment.
This is my ignorance of go style, but is it better to have this check for a false, or is it more readable to have a var as something like append_default_flags and make it affirmative.
There was a problem hiding this comment.
Go convention is !x rather than x == false (staticcheck flags it as S1002), and local variables are camelCase rather than snake_case. I've pushed a small commit that renames to suppressDefaultFlags and flips the condition to !suppressDefaultFlags.
Keeping the negative name is fine here since it mirrors the schema field; an affirmative appendDefaultFlags would read nicely too but then it no longer matches what's in the YAML, so I left it as-is.
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Thanks, this addresses everything from the earlier review: default_flags: [--upgrade] is back, TestUvUpdate still asserts the no-arg case, and the new suppresses_default_flags mechanism handles the targeted upgrade cleanly. Both uv sync --upgrade and uv sync --upgrade-package <pkg> now come out correctly.
I pushed one tiny commit on top to satisfy golangci-lint (camelCase local + !x instead of x == false), no logic change.
2 participants
When updating a specific package, need to use --upgrade-package instead of just --update. Adding an extra test case to handle both with and without package.
Fixes #20